@fmvilas oh crap, now we have a problem with automerging 🤔

Any idea what's happening?

@fmvilas yeah, we now require 3 approvers

Oh damn true 😂 I just added @asyncapi-bot to an exception list. Shall we try here again?

@fmvilas oh, I had no idea we can do such a bypass

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

Me neither, I was just playing around with the UI and found it by luck 😅

merging is still blocked 🤔 and any changes in settings should work out of the box, even without closing and opening a PR

Yeah, I see. What I did is to allow asyncapi-bot to bypass having to create a PR. That's a different thing.

So my proposal then is that we don't enforce it but we still commit to the rule "manually". \cc @dalelane @char0n @smoya

So my proposal then is that we don't enforce it but we still commit to the rule "manually". \cc @dalelane @char0n @smoya

I'm fine with that!

Yeah, I see. What I did is to allow asyncapi-bot to bypass having to create a PR. That's a different thing.

So my proposal then is that we don't enforce it but we still commit to the rule "manually". \cc @dalelane @char0n @smoya

Oh, but this wouldn't help anyway, right? Automerge won't happen ever unless 2 extra approves happen (2 admins), meaning only "manual" merge by a human (via /rtm or whatever) will let the PR to be merged.

Not ideal at all IMHO. Would it make sense to rather disable this:

Admins could still commit to the rule of 3 approvals for all PRs, but still, those created by the bot will be automerged.
Cons: admins could merge ""accidentally"" PRs without approvals from other admins.

Tradeoffs, tradeoffs everywhere!

@smoya No, I can put it back to only 1 approver required and it would merge with no problem, like everywhere else. But we need to honor the rule of 3 approvers to merge a PR, even though it's not enforced by GitHub. This makes me think now that maybe the /rtm command could be checking for 3 approvers minimum. What do you think, @derberg?

@smoya No, I can put it back to only 1 approver required and it would merge with no problem, like everywhere else. But we need to honor the rule of 3 approvers to merge a PR, even though it's not enforced by GitHub. This makes me think now that maybe the /rtm command could be checking for 3 approvers minimum. What do you think, @derberg?

But afaik by disabling the option I mentioned still GH will enforce that, except for code owners/admins; that's the trade-off.

Yes, the action that we use for merging has a support for MERGE_REQUIRED_APPROVALS that we can set to 3.
2 disadvantages:

• It would mean we would have to modify the workflow, and it would no longer receive updates from central location in the future
• This would not be very transparent. People would try to merge with command, but it would not work, without providing context

Easier would be to create additional 2 bot accounts 🤣

Easier would be to create additional 2 bot accounts 🤣

LOL

It would mean we would have to modify the workflow, and it would no longer receive updates from central location in the future

Can't we make the action to take this value from a Github Secret in the repo? It would be like setting an environment variable for this repo only. We can default to 1 if this secret is not found.

This would not be very transparent. People would try to merge with command, but it would not work, without providing context

What about the command shows that it didn't merge because of that reason? I mean, it leaves a comment in the PR.

Can't we make the action to take this value from a Github Secret in the repo? It would be like setting an environment variable for this repo only. We can default to 1 if this secret is not found.

neat workaround! should work, we would have org-wide secret set to 1 and in this repo and others overwritten

What about the command shows that it didn't merge because of that reason? I mean, it leaves a comment in the PR.

yeah, I checked this action. It do not offer proper information in the output. From output we can only learn that merge did not work, but why exactly? failing tests? missing approvals? - this info is not available. Maybe they could enable it, but this will definitely require a contribution.

we could do our own query:

query { 
  repository(owner:"asyncapi", name:"spec") {
    pullRequest(number:851) {
      reviewDecision
    }
  }
}

/*
####Review Decision type####
CHANGES_REQUESTED
Changes have been requested on the pull request.

APPROVED
The pull request has received an approving review.

REVIEW_REQUIRED
A review is required before the pull request can be merged.
*/

for example this PR returns REVIEW_REQUIRED even though bot approved it. So my assumption is, that it is because API knows all 3 approvals are missing. Should be good enough to put a comment "not all approvals are in place"

maybe https://github.com/asyncapi/.github/blob/master/.github/workflows/automerge-for-humans-add-ready-to-merge-or-do-not-merge-label.yml#L29 should just be extended. We already handle "conflicts" there and post comments, so missing approvals should be handled there too I guess.

Alright, so now the question is, who volunteers to do it? 😄 I don't think it should be @derberg because he's quite busy with the conference. Any volunteers?

@fmvilas maybe just open an issue in .github and till then we will just follow kinda written agreement that we will respect minimum of 3 approvers (as we did anyway before onboarding new codeowners)

Done: asyncapi/.github#190

/rtm

🎉 This PR is included in version 3.0.0-next-major-spec.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

🎉 This PR is included in version 2.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀